Adding Resolution Paths during appdomain creation (Compatibility Isuue)#404
Conversation
| public void SetupHost() | ||
| { | ||
| if (this.AppDomainCreationDisabledInRunSettings()) | ||
| List<string> resolutionPaths = this.GetResolutionPaths(this.sourceFileName, VSInstallationUtilities.IsCurrentProcessRunningInPortableMode()); |
There was a problem hiding this comment.
Please add test for ResolutionPaths being set.
| } | ||
|
|
||
| this.targetFrameworkVersion = this.GetTargetFrameworkVersionString(this.sourceFileName); | ||
| // Adding extensions folder to resolution paths |
There was a problem hiding this comment.
Nit: this is runner location(OM assembly location).
| List<string> resolutionPaths = this.GetResolutionPaths(this.sourceFileName, VSInstallationUtilities.IsCurrentProcessRunningInPortableMode()); | ||
|
|
||
| // Check if user specified any runsettings | ||
| MSTestAdapterSettings adapterSettings = MSTestSettingsProvider.Settings; |
There was a problem hiding this comment.
Move the declaration near to usage.
| // Check if user specified any runsettings | ||
| MSTestAdapterSettings adapterSettings = MSTestSettingsProvider.Settings; | ||
|
|
||
| if (resolutionPaths != null && resolutionPaths.Count > 0) |
There was a problem hiding this comment.
This should not be happen. Even if it is empty or null, We should add below two paths.
| } | ||
| catch (Exception exception) | ||
| { | ||
| if (EqtTrace.IsErrorEnabled) |
There was a problem hiding this comment.
Nit: Remove if check for error scenarios.
| { | ||
| if (EqtTrace.IsErrorEnabled) | ||
| { | ||
| EqtTrace.Error(exception); |
There was a problem hiding this comment.
Nit: Add more details while logging the exception. At line 190 too.
| var configFile = this.GetConfigFileForTestSource(this.sourceFileName); | ||
| AppDomainUtilities.SetConfigurationFile(appDomainSetup, configFile); | ||
|
|
||
| this.domain = this.appDomain.CreateDomain("TestSourceHost: Enumering assembly", null, appDomainSetup); |
There was a problem hiding this comment.
Change the name of appdomain to sourcefilename.
|
|
||
| // Case when Child-appdomain was created successfully, update appbase and set assembly resolver on it | ||
| else if (this.domain != null) | ||
| if (this.domain != null) |
There was a problem hiding this comment.
Change this to inverted if IsAppDomainCreationDisable.
|
|
||
| // Log error when child-appdomain was expected to be created but wasn't created. | ||
| else | ||
| else if (!this.AppDomainCreationDisabledInRunSettings()) |
There was a problem hiding this comment.
This is not required. Are DisableAppDomain settings from runsettings only once.
| try | ||
| { | ||
| var additionalSearchDirectories = | ||
| adapterSettings.GetDirectoryListWithRecursiveProperty(this.domain.SetupInformation.ApplicationBase); |
There was a problem hiding this comment.
this.domain.SetupInformation.ApplicationBase value should be testsource directory.
There was a problem hiding this comment.
Reuse additionalSearchDirectories.
| try | ||
| { | ||
| this.parentDomainAssemblyResolver = new AssemblyResolver(resolutionPaths); | ||
| this.parentDomainAssemblyResolver.AddSearchDirectoriesFromRunSetting(adapterSettings.GetDirectoryListWithRecursiveProperty(null)); |
There was a problem hiding this comment.
GetDirectoryListWithRecursiveProperty should take test source directory.
| Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "DoNotParallelizeTestProject", "test\E2ETests\TestAssets\DoNotParallelizeTestProject\DoNotParallelizeTestProject.csproj", "{8080DE48-CFD9-4F33-908A-8B71108CA223}" | ||
| EndProject | ||
| Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "CompatTestProject", "test\E2ETests\TestAssets\CompatTestProject\CompatTestProject.csproj", "{2D2C5B73-F1F1-47C8-BC5C-A172E9BB3D16}" | ||
| Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "CompatTestProject", "test\E2ETests\TestAssets\CompatTestProject\CompatTestProject.csproj", "{2D2C5B73-F1F1-47C8-BC5C-A172E9BB3D16}" |
There was a problem hiding this comment.
Consider having separate sln for test assets.
| this.SetContext(sourceFileName); | ||
| } | ||
|
|
||
| public AppDomain AppDomain |
| { | ||
| List<string> resolutionPaths = this.GetResolutionPaths(this.sourceFileName, VSInstallationUtilities.IsCurrentProcessRunningInPortableMode()); | ||
|
|
||
| EqtTrace.Info("DesktopTestSourceHost.SetupHost(): Creating assembly resolver with resolution paths {0}.", string.Join(",", resolutionPaths.ToArray())); |
|
|
||
| if (adapterSettings != null) | ||
| { | ||
| try |
| var mockRunSettings = new Mock<IRunSettings>(); | ||
| mockRunSettings.Setup(rs => rs.SettingsXml).Returns(runSettingxml); | ||
|
|
||
| StringReader stringReader = new StringReader(runSettingxml); |
| <IsPackable>false</IsPackable> | ||
| <AppendTargetFrameworkToOutputPath>false</AppendTargetFrameworkToOutputPath> | ||
| <OutputPath>$(TestFxRoot)artifacts\TestAssets\ComponentTests</OutputPath> | ||
| <SignAssembly>true</SignAssembly> |
There was a problem hiding this comment.
Remove this if not required?
There was a problem hiding this comment.
Required. SampleFrameworkExtensions also has it. PlatformServices.Desktop.Component.Tests throws strong name error if key.snk is not referenced in TestProjectForAssemblyResolution.
|
@dotnet-bot Test Windows / Debug Build please |
Scenario which was broken -
If suppose TestProject has a config file having assembly binding redirect of System.Runtime to 4.1. Our TestAdapter which has a dependency of System.Runtime of 4.0, uses the above config file and tries to load System.Runtime 4.1. Since assembly resolver was not set at the time of loading of adapter, it is unable to find 4.1 version of assembly and bombs.
Fix - Adding resolution paths at the time of creation of appdomains.
Validations-